-
-
Notifications
You must be signed in to change notification settings - Fork 21.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
BasisUniversal: Fix artifacts on images with resolutions not divisible by 4 #89426
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me.
I do not agree with this approach. Unless I am misreading the code, it is silently resizing the image by a few pixels which will cause blurry heavily antialiasing artifacts on any text, lines or pixel art. The user will be unaware of why their art is getting corrupted when basisu is used. There is no requirement I can see that textures are rounded to a multiple of 4: If we are unable to implement this algorithm or there is not enough time to implement for 4.3, it would be better to fail the import (with a user visible warning in the import dock IMHO) than to silently resize it, and perhaps have a dropdown where the user can manually choose to resize to 4 if they want. |
This comment was marked as outdated.
This comment was marked as outdated.
d840e9d
to
094b608
Compare
The processing is now done without upscaling, but with a slight variation of the smearing algorithm from Godot's etcpak implementation. |
094b608
to
8512656
Compare
34e5d3f
to
f1deb20
Compare
f1deb20
to
b0bbfcb
Compare
CC @lyuma |
b0bbfcb
to
86fb006
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed this one. The algorithm for resizing, padding and smearing pixels seems correct for cases where w>2 and h>2.
I will admit I am not familiar with the w=1 and w=2 cases: I would think those also need to be padded to a multiple of 4, but maybe it is handled by basisu already so we would need to test and maybe fix separately if needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving for good luck.
86fb006
to
77719bd
Compare
Thank you for the review! Images with small resolutions would trigger the "BasisUniversal cannot unpack level 1" error. As it turns out, this bug would also happen to all textures without mipmaps due to incorrect logic for checking the mipmap count, which I've fixed now. |
77719bd
to
e5bacbf
Compare
e5bacbf
to
c714900
Compare
Thanks! |
Fixes artifacts on mipmaps of BasisU-compressed images whose resolutions aren't multiples of 4.
MRP: Basisu_not_mul_4_test.zip
(Requires reimporting the image).